-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Project Design Handoff #48
base: master
Are you sure you want to change the base?
Conversation
…svg and png files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done, Lina! This was a huge project and you got a lot of the major elements in! It looks like there is just some tidying up in some places with images and font weights etc, and some media queries to do. The overview section is definitely one of the most complicated - a tip is to put everything within borders as you work so that you can see what is happening and how things are moving on the screen. Also putting all the vector shapes within their own containers and making the containers position: relative
and the shapes position: absolute
. Good luck with it and well done!
|
||
export const App = () => { | ||
return ( | ||
<div> | ||
Find me in src/app.js! | ||
<div className="landingPage"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious as to why you need a landing page classname if you’re not going to do any additional styling to it?
@@ -1,13 +1,22 @@ | |||
* { | |||
box-sizing: border-box; | |||
/* margin: 0; */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious as to why you commented out margin: 0
here and wonder if it affects your overspill on the x axis?
{/* <div className="navBar"> */} | ||
<img className="logo" src={LogoSmallonLightBackground} alt="Raise Studio logo" /> | ||
<div className="menu"> | ||
<img className="hamburgerMenu" src={HamburgerMenu} alt="Menu icon - click to expand navigation menu" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header looks great with the icon and the hamburger icon! Not much more to do to make it responsive - to create a tablet and desktop version, I don’t know if its best practice but I created a desktop version div within the component and just put display: none
for the mobile version (and added the two more buttons needed to the mobile version for tablet).
.intro { | ||
display: flex; | ||
flex-direction: column; | ||
height: 100vh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the structure doesn't quite work here - I also put everything in one wrapper with display: flex
and flex-direction: column
, but without height: 100vh
; maybe this would help keep this section separate from the surrounding sections? Because I think otherwise it wants to take up the whole viewport height.
<h2>Barre Basic</h2> | ||
<p>Our Beginner’s Class will give you a solid foundation on your...</p> | ||
<button type="button" className="readMoreBtn">Read More</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart to add this text for screen readers to work, instead of just using the text from the picture!
@@ -0,0 +1,21 @@ | |||
.quizContainer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job making this quiz section responsive!
} | ||
|
||
|
||
.vectorShape { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a hard time keeping these vector shapes in the right place! I got some advice to put them in their own parent container, and make the container position: relative
and the shape position: absolute
. (So the foreground shape is positioned somewhere precisely within its parent container).
<span>I have read and understood the | ||
<button type="button" className="registrationBtn"> Terms & Conditions</button> and | ||
<button type="button" className="registrationBtn"> Privacy Policy</button> | ||
</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you used span
here instead of div
to show it’s a section? The span
element is an inline-level element used for styling or grouping inline content. So it typically groups code on one line rather than a block of code with multiple lines.
</label> | ||
</div> | ||
<span>Join Raise Studio</span> | ||
<input type="submit" value="submit" className="joinRaiseStudioBtn" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you used input
here instead of a button
element, like below:
<button type="submit" className="joinbtn">Join</button> ?
@@ -0,0 +1,26 @@ | |||
.footerContainer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job with the footer! All the elements are there!
No description provided.